Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rename InvRing + small changes to invariant theory #3442

Merged
merged 3 commits into from
Mar 1, 2024
Merged

Rename InvRing + small changes to invariant theory #3442

merged 3 commits into from
Mar 1, 2024

Conversation

joschmitt
Copy link
Member

@joschmitt joschmitt commented Feb 27, 2024 •
edited
Loading

Rename:

  • InvRing to FinGroupInvarRing
  • RedGrpInvRing to RedGroupInvarRing
  • TorGrpInvRing to TorGroupInvarRing

The renamings do not interfere with the book and are no breaking changes since they are either internal or in experimental.

I made further small internal changes to the experimental invariant theory (which do not interfere with the book).
This contains the changes from #3469, I will rebase once that is merged.

@joschmitt joschmitt mentioned this pull request Feb 27, 2024
Merged
@thofma
Copy link
Collaborator

thofma commented Feb 27, 2024

Just mentioning here, that Group in most type names is often not abbreviated to Grp: PermGroup, PcGroup, MatrixGroup, AbGroup, but it seems invariant theory settled on Grp.

@joschmitt joschmitt marked this pull request as draft February 27, 2024 16:00
@joschmitt
Copy link
Member Author

I make this a draft as it creates conflicts with #3443. Also, we should settle the Grp vs Group question first. (And maybe Inv vs Invar while we are at it.)

@wdecker
Copy link
Collaborator

wdecker commented Feb 27, 2024

@joschmitt Sorry, I have seen your PR only now.

@joschmitt
Copy link
Member Author

@joschmitt Sorry, I have seen your PR only now.

Don't worry. This is only internal changes and can wait.

@fingolfin
Copy link
Member

I am neutral on the Grp vs Group thing here, and if @thofma prefers to have Group, I am happy with that, too.

Regarding Inv vs Invar I'd prefer Invar, as InvRing could also stand for "involutory ring". But I am happy either way, for me the most important part is that we achieve "local" consistency, that is: the types for invariant rings for finite groups and for reductive groups are getting aligned.

@joschmitt
Copy link
Member Author

Okay, I will change everything to *GroupInvarRing once #3443 is through (and I checked that it does not interfere with the book...)

@joschmitt joschmitt mentioned this pull request Feb 29, 2024
Merged
@joschmitt joschmitt marked this pull request as ready for review February 29, 2024 15:35
@joschmitt
Copy link
Member Author

This is good to go from my side, but waiting for #3469.

fingolfin
fingolfin approved these changes Feb 29, 2024
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3469 is in, so now a conflict needs to be resolved. Otherwise seems fine to me, thanks

@joschmitt
Small fixes to experimental invariant theory
9edb03e 
* use `monomials_of_degree`
* `NullConeIdeal` -> `null_cone_ideal`
* remove unnecessary exports
@joschmitt
Rename invariant ring types
14c04d2 
`InvRing` -> `FinGroupInvarRing`
`RedGrpInvRing` -> `RedGroupInvarRing`
`TorGrpInvRing` -> `TorGroupInvarRing`
@joschmitt
Add fields for fundamental invariants and presentation as affine algebra
4e1c994
@joschmitt
Copy link
Member Author

This is good to go now (once CI approves).

@thofma thofma enabled auto-merge (squash) March 1, 2024 07:23
@codecov Codecov
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Merging #3442 (4e1c994) into master (fafa084) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 95.48%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3442      +/-   ##
==========================================
- Coverage   81.93%   81.93%   -0.01%     
==========================================
  Files         563      563              
  Lines       75550    75541       -9     
==========================================
- Hits        61901    61892       -9     
  Misses      13649    13649
Files Coverage Δ
...imental/InvariantTheory/src/TorusInvariantsFast.jl 100.00% <100.00%> (ø)
src/InvariantTheory/affine_algebra.jl 98.80% <100.00%> (ø)
src/InvariantTheory/fundamental_invariants.jl 97.84% <100.00%> (ø)
src/InvariantTheory/primary_invariants.jl 96.89% <100.00%> (ø)
src/InvariantTheory/types.jl 100.00% <100.00%> (ø)
src/InvariantTheory/secondary_invariants.jl 99.53% <85.71%> (ø)
...xperimental/InvariantTheory/src/InvariantTheory.jl 90.79% <95.83%> (-0.40%) ⬇️
src/InvariantTheory/invariant_rings.jl 90.96% <92.59%> (ø)
src/InvariantTheory/iterators.jl 91.33% <89.47%> (ø)

@thofma thofma merged commit 98bdf19 into oscar-system:master Mar 1, 2024
25 of 26 checks passed
@joschmitt joschmitt deleted the js/invars branch March 1, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joschmitt @thofma @wdecker @fingolfin